a good practice is to occasionally rotate the secret, https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_secretsmanager/RotationSchedule.html

Makes sense! Will the password environment variable in the API container be automatically updated when the secret is rotated?

yes, i believe that's the best feature of secrets manager. here's a blog on how to setup https://aws.amazon.com/blogs/security/how-to-rotate-amazon-documentdb-and-amazon-redshift-credentials-in-aws-secrets-manager/

In the article you linked, the application retrieves the password directly from Secrets Manager at runtime. However, the Agora API container currently retrieves the password from an environment variable, which is set by the ServiceStack during deployment. If the secret is retrieved at deployment time, then would the API container environment variable only be updated when the stack is redeployed (and not automatically be updated when the secret is rotated)?

If that's right, then to allow us to use automatic password rotation, I think we'd need to update the API to retrieve the password directly from Secrets Manager (and refetch the credentials without restarting the container, as described here) at runtime, rather than getting the password from an environment variable. What do you think?

ahh, yes you are correct. getting the secret from an env var will not work with secrets rotation. refactoring the API to get secrets directly from the secrets manager would be a better approach because is more secure and works with secrets rotation. I guess we can leave out secrets rotation at this time. I'll let you guys decide whether you want to refactor Agora to support this use case. I would highly recommend it though.

duplicate parameter?

Will remove in the next commit

the convention used in other files for construct_id is camel case. Also i don't think you don't need to get fancy here, you can just set to something like "DocDbMasterPassword"

name parameter does not seem like it's required? if a name parameter is not required I would leave it out and let AWS name the resource. You can always dynamically get a resource name from its Ref.

27017 is used multiple times in this file. would it make sense to create a variable for this?

hmm, it looks like we add the security_group defined here to every container so how about we move the security_group to the ServiceProps class? It can just be added to the container_security_groups object as a default SG?

If we create the SecurityGroup in ServiceProps, then I believe ServiceProps would need to inherit from Construct, which would result in duplicate code between the props and the stack class. What do you think about continuing to create security_group in ServiceStack, so that the resource creation only happens in one class?

What do you think about continuing to create security_group in ServiceStack, so that the resource creation only happens in one class?

Not sure exactly what you mean but if you mean define a list of service groups in app.py then pass it to ServiceProps object then I think that will work as well.

isn't this a dup of APP_VERSION?

They're very similar, but not quite the same (e.g. 4.0.0 for APP_VERSION vs agora/v4.0.0.0 for TAG_NAME). @sagely1 would know better, but I believe separate variables were created because the GitHub tag format was in flux. However, with that decision made and the format so similar, they should probably be collapsed to just one environment variable, but that will require work in the sage-monorepo first!

We have settled on semantic versioning after some discussion. TAG_NAME will be a string of a format like so:
agora/vX.X.X with a potential suffix such as -rc.X so agora/vX.X.X-rc.X. I think we can just change this to get a tag_name variable instead. https://semver.org/

Great! I've opened a ticket to track the work, since we'll need to update the sage-monorepo code first.

We may need these to be https considering that the app will be served over https

Sounds good, I can change that in the next commit